-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: fix the system message in simple-chatbot example #1394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yukuanj thank you for this contribution!
I tested out and verified that the example you show is indeed broken in the same way for me - however, checking out your branch here doesn't consistently resolve the issue for me. Sometimes it works, sometimes it doesn't.
It still ends up wrapping the tool call in backticks
- to be less brittle, I think we'd want to update process_llm_response
in this file instead to handle the case when json ...
is sent back?
@felixweinberger Thank you so much for reviewing and testing out this pull request. I strongly agree with your point that we should update What I changedWe only attempt a tool call when the response is a single JSON object, optionally wrapped in a fenced code block.
How it worksA small regex strips a single fenced block (with or without the word 'json') and then try json.loads(). Example session
Looking forward to your feedback! |
@felixweinberger Hi Felix! I’ve updated the code so that it now passes the checks. Could you please take a look when you get a chance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes, lgtm!
In the simple-chatbot example, I have updated the system message to explicitly suppress the triple backticks fences output by the LLM. Otherwise, the triple backticks fences would prevent the tool calling.
Motivation and Context
When I executed the simple-chatbot example, I observed:
This is not actually calling the MCP tool
list_table
, because the triple backticks fences make the response unparsable byjson.loads()
.Therefore, I add 'without the triple backticks fences' explicitly in the system message. Now the LLM is outputting parsable json, and the tools can be called correctly and smoothly.
How Has This Been Tested?
Tested with the default LLM model with default parameters in the example:
Breaking Changes
No breaking changes.
Types of changes
Checklist